Add support for dynamic Dashboards URL via cluster settings by praveenvenkat06 · Pull Request #6275 · opensearch-project/security

praveenvenkat06 · 2026-07-01T10:52:12Z

Description

Enhancement — Introduces a new dynamic cluster setting plugins.security.dashboards.dashboards_url that allows operators to update the OpenSearch Dashboards URL used in the SAML authentication flow at runtime, without modifying the security index or reloading configuration.

Old behavior: The Dashboards URL (kibana_url) was read once from the SAML authenticator's security-index configuration and required a config reload to change.

New behavior: The URL is resolved dynamically at every call site using the following precedence:

Dynamic cluster setting (plugins.security.dashboards.dashboards_url) — if set and non-empty
Fallback to kibana_url in the security configuration

The SAML settings cache (Saml2SettingsProvider) is automatically invalidated when the setting value changes, so updates take effect immediately with no restart required.

Issues Resolved

[6015]

Testing

Manual testing is done and dynamic Dashboards URL changes are reflected at runtime. Unit tests have been added for the implementation.

Check List

New functionality includes testing
New functionality has been documented - Will be done once the PR gets approved
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created - Will be done once the PR gets approved
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-01T10:53:02Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit afbf218.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java	183	medium	The SAML SP Assertion Consumer Service URL is constructed from a dynamically controllable cluster setting (SECURITY_DASHBOARDS_URL). A malicious cluster admin can redirect SAML assertions to an attacker-controlled endpoint by updating this setting at runtime, enabling SAML token hijacking without touching the security index.
src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java	410	medium	The ACS endpoint resolution in getAbsoluteAcsEndpoint() uses the dynamically-supplied dashboards URL as a base URI. If the cluster setting is updated to an attacker-controlled host, relative ACS endpoints will be resolved against it, redirecting SAML responses to that host — a form of SSRF/open-redirect in the SAML flow.
src/main/java/org/opensearch/security/auth/http/saml/HTTPSamlAuthenticator.java	181	low	getDashboardsUrl() logs the full URL (including any embedded credentials in the form http://user:pass@host) at debug level. If the Dashboards URL ever contains credentials, they would be written to logs. The setting is marked Sensitive in DashboardsUrlSetting but that protection does not extend to these debug log statements.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 2 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-07-01T10:53:47Z

PR Reviewer Guide 🔍

(Review updated until commit `33b24e7`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Possible Race Condition In `getCached()`, `dashboardsUrlSupplier.get()` is invoked twice: once inside `isDashboardsUrlChanged()` and again when caching the new value in `lastUsedDashboardsUrl`. If the dynamic setting changes between these two calls, the cache will store a URL that differs from the one actually used to build `cachedSaml2Settings`, causing subsequent change detection to miss the update or trigger an unnecessary rebuild. Capture the value once and reuse it for both the comparison and the cache update. Saml2Settings getCached() throws SamlConfigException { Instant tempLastUpdate = null; if (this.metadataResolver instanceof RefreshableMetadataResolver && this.isUpdateRequired()) { this.cachedSaml2Settings = null; tempLastUpdate = ((RefreshableMetadataResolver) this.metadataResolver).getLastUpdate(); } if (this.isDashboardsUrlChanged()) { this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } return this.cachedSaml2Settings; } Thread Safety `getCached()` mutates `cachedSaml2Settings`, `metadataUpdateTime`, and now `lastUsedDashboardsUrl` without synchronization. Previously only metadata refresh could invalidate the cache; now every request may invalidate it based on an external dynamic setting. Concurrent SAML authentication requests could observe partially-updated state (e.g., cache cleared but not repopulated, or `lastUsedDashboardsUrl` updated before `cachedSaml2Settings`), producing settings inconsistent with the current dashboards URL. Saml2Settings getCached() throws SamlConfigException { Instant tempLastUpdate = null; if (this.metadataResolver instanceof RefreshableMetadataResolver && this.isUpdateRequired()) { this.cachedSaml2Settings = null; tempLastUpdate = ((RefreshableMetadataResolver) this.metadataResolver).getLastUpdate(); } if (this.isDashboardsUrlChanged()) { this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } return this.cachedSaml2Settings; } Global Mutable State `GuiceHolder.setDashboardsUrlSetting` exposes a mutable static field intended for cross-component wiring, but it is publicly settable (as demonstrated by tests calling it with `null`). Any caller may overwrite or clear the reference at runtime, breaking SAML authentication silently since `getDashboardsUrl()` will fall back to `kibanaRootUrl` without error. Consider restricting visibility or making the setter one-shot/package-private. public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; }

github-actions · 2026-07-01T10:54:17Z

PR Code Suggestions ✨

Latest suggestions up to 33b24e7

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid race by reading supplier once `dashboardsUrlSupplier.get()` is invoked twice (once in `isDashboardsUrlChanged()` and again here) and its value may change between the calls, causing a race where `lastUsedDashboardsUrl` is set to a value different from the one actually used by `this.get()`. Capture the value once and pass it consistently, or read it before `get()` and reuse. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] -if (this.isDashboardsUrlChanged()) { +String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); +if (!Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 7 __ Why: Valid concern: calling the supplier twice could lead to inconsistent state between `lastUsedDashboardsUrl` and the actual URL used in `get()`. Capturing the value once is a meaningful correctness improvement.	Medium
General	Remove Sensitive property from URL setting Marking the setting as `Sensitive` will cause it to be filtered from settings APIs and logs, but a Dashboards URL is not sensitive information and users may reasonably expect to view/verify it via `GET _cluster/settings`. Consider removing `Property.Sensitive` unless intentional. src/main/java/org/opensearch/security/setting/DashboardsUrlSetting.java [31-33] private static Setting<String> getSetting() { - return Setting.simpleString(SETTING, Setting.Property.NodeScope, Setting.Property.Dynamic, Setting.Property.Sensitive); + return Setting.simpleString(SETTING, Setting.Property.NodeScope, Setting.Property.Dynamic); } Suggestion importance[1-10]: 5 __ Why: A Dashboards URL is generally not sensitive; marking it as Sensitive would filter it from APIs and hinder operator visibility. This is a reasonable design consideration though possibly intentional.	Low
	Fail fast on missing Dashboards URL Returning a relative `acsEndpoint` as an absolute endpoint when the Dashboards URL is missing will produce an invalid SAML AssertionConsumerService URL and can cause downstream authentication failures that are hard to diagnose. Prefer throwing an explicit exception so the misconfiguration surfaces clearly rather than silently returning an invalid value. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-412] String dashboardsUrl = dashboardsUrlSupplier.get(); if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { - log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); - return acsEndpoint; + throw new RuntimeException( + "Dashboards URL is not configured; cannot resolve relative acsEndpoint: " + acsEndpoint + ); } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 5 __ Why: Failing fast on misconfiguration provides better diagnostic feedback than silently returning an invalid endpoint, but this is a debatable design choice and the current behavior logs an error.	Low
	Clear static setting reference on close The static `dashboardsUrlSetting` field in `GuiceHolder` is mutable global state that will leak across tests and, in production, could retain stale references if the plugin is re-initialized. Consider clearing it on `close()` to avoid stale state and ensure test isolation is not relied upon by resetting to null in the setter's teardown paths. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2599] private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; +@Override +public void close() { + dashboardsUrlSetting = null; +} + Suggestion importance[1-10]: 4 __ Why: Clearing static state on close is a reasonable hygiene improvement for test isolation and to avoid stale references, but its impact is limited since the plugin lifecycle typically doesn't reinitialize in production.	Low

Previous suggestions

Suggestions up to commit afbf218

Category	Suggestion	Impact
Possible issue	Capture URL once to avoid race The `dashboardsUrlSupplier.get()` is invoked twice (once in `isDashboardsUrlChanged()` and again here) and the value may change between calls, causing `lastUsedDashboardsUrl` to be stored with a value different from the one actually used to build the settings. Capture the URL once before the change check and reuse it to keep the cached settings and `lastUsedDashboardsUrl` consistent. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] -if (this.isDashboardsUrlChanged()) { +String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); +if (!Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { + log.debug( + "Dashboards URL has changed from '{}' to '{}'; SAML settings cache will be refreshed", + this.lastUsedDashboardsUrl, + currentDashboardsUrl + ); this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 7 __ Why: Valid concern: `dashboardsUrlSupplier.get()` is called twice and could return different values, leading to inconsistent state between the cached settings and `lastUsedDashboardsUrl`. The refactor improves correctness.	Medium
General	Fail fast on missing Dashboards URL Returning the raw relative `acsEndpoint` when the Dashboards URL is missing will silently produce an invalid absolute ACS URL used downstream in SAML request construction, likely leading to broken redirects rather than a clear failure. Prefer throwing a clear configuration exception so the misconfiguration surfaces immediately. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-412] String dashboardsUrl = dashboardsUrlSupplier.get(); if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); - return acsEndpoint; + throw new RuntimeException("Dashboards URL is not configured; cannot resolve relative acsEndpoint: " + acsEndpoint); } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 5 __ Why: Valid point that silently returning the relative endpoint may cause obscure downstream failures, though the behavior change could also break flows that previously tolerated missing URLs. Moderate impact.	Low
General	Restrict visibility of static mutator Exposing a public static setter for `dashboardsUrlSetting` allows arbitrary callers (or leaked test state) to overwrite plugin-wide state. Consider restricting the setter to package-private or `@VisibleForTesting`, so production code paths cannot mutate the singleton after initialization. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2650-2656] public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } -public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { +@VisibleForTesting +static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; } Suggestion importance[1-10]: 4 __ Why: Reasonable encapsulation improvement, but the setter is called from `createComponents` in the same class, so restricting it to package-private is feasible but has minor impact.	Low

Suggestions up to commit 008a6b7

Category	Suggestion	Impact
Possible issue	Avoid race in URL change detection There is a race condition between reading `currentDashboardsUrl` for comparison in `isDashboardsUrlChanged()` and re-reading it again after `this.get()` executes. If the supplier value changes between these calls, `lastUsedDashboardsUrl` may be updated to a value that does not match what was actually used to build `cachedSaml2Settings`, leading to stale settings not being detected on the next call. Capture the value once and reuse it. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-138] - if (this.isDashboardsUrlChanged()) { + String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); + if (!Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { + log.debug( + "Dashboards URL has changed from '{}' to '{}'; SAML settings cache will be refreshed", + this.lastUsedDashboardsUrl, + currentDashboardsUrl + ); this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } Suggestion importance[1-10]: 6 __ Why: Valid concern about a potential race condition where the supplier value could change between the `isDashboardsUrlChanged()` check and the re-read after `this.get()`, potentially causing stale settings to be undetected. Impact is minor since dashboards URL changes are rare.	Low
Possible issue	Guard against null Dashboards URL If `dashboardsUrlSupplier.get()` returns null (e.g., cluster setting cleared and legacy config also missing), `new URI(null)` will throw `NullPointerException` rather than the handled `URISyntaxException`, breaking the fallback flow. Add an explicit null/empty check and log or fall back gracefully. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-408] - return new URI(this.kibanaRootUrl).resolve(acsEndpointUri).toString(); - } else { String dashboardsUrl = dashboardsUrlSupplier.get(); + if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { + log.error("Dashboards URL is not configured; cannot resolve relative ACS endpoint: {}", acsEndpoint); + return acsEndpoint; + } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); - } Suggestion importance[1-10]: 6 __ Why: Correctly identifies that `new URI(null)` throws NPE rather than the caught URISyntaxException, which could break error handling. Adding a null check improves robustness of the fallback flow.	Low
General	Avoid leaking static mutable global state Using a static mutable holder to bridge plugin state into authenticators creates hidden global state that leaks across test cases and node restarts. If a plugin instance is re-created (e.g., in tests or reloads), the previous instance's setting reference remains, potentially serving stale values. Consider passing the setting/supplier through authenticator construction, or at least clearing it during plugin shutdown/close. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2650-2656] public static OpensearchDynamicSetting<String> getDashboardsUrlSetting() { return dashboardsUrlSetting; } public static void setDashboardsUrlSetting(OpensearchDynamicSetting<String> setting) { dashboardsUrlSetting = setting; } + public static void clearDashboardsUrlSetting() { + dashboardsUrlSetting = null; + } + Suggestion importance[1-10]: 3 __ Why: Valid design observation about static mutable state, but the `improved_code` merely adds a `clearDashboardsUrlSetting()` method without changing the fundamental design; it's a marginal improvement.	Low

Suggestions up to commit 410b3dd

Category	Suggestion	Impact
Possible issue	Guard against null Dashboards URL If `dashboardsUrlSupplier.get()` returns null (e.g., neither the dynamic setting nor `kibana_url` is configured), `new URI(null)` will throw a `NullPointerException` which is not caught by the `URISyntaxException` handler below. Validate the supplier result and log/return a meaningful error to avoid unexpected NPE at runtime. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [407-408] String dashboardsUrl = dashboardsUrlSupplier.get(); +if (dashboardsUrl == null \|\| dashboardsUrl.isEmpty()) { + log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); + return acsEndpoint; +} return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); Suggestion importance[1-10]: 6 __ Why: Valid concern: if both dynamic setting and `kibana_url` are unset, `new URI(null)` would throw an NPE not caught by `URISyntaxException`. The suggestion adds a reasonable null/empty guard.	Low
General	Fix concurrency in cache invalidation `getCached()` is not synchronized, but `cachedSaml2Settings`, `metadataUpdateTime`, and `lastUsedDashboardsUrl` can be accessed concurrently by multiple request threads. Concurrent invalidation and re-population may cause inconsistent state (e.g., stale `lastUsedDashboardsUrl` paired with new settings). Synchronize the method or make the fields volatile with proper atomic update semantics. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [129-137] -if (this.isDashboardsUrlChanged()) { - this.cachedSaml2Settings = null; +synchronized (this) { + if (this.isDashboardsUrlChanged()) { + this.cachedSaml2Settings = null; + } + + if (this.cachedSaml2Settings == null) { + String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); + this.cachedSaml2Settings = this.get(); + this.metadataUpdateTime = tempLastUpdate; + this.lastUsedDashboardsUrl = currentDashboardsUrl; + } } -if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); - this.cachedSaml2Settings = this.get(); - this.metadataUpdateTime = tempLastUpdate; - this.lastUsedDashboardsUrl = currentDashboardsUrl; -} - Suggestion importance[1-10]: 5 __ Why: Concurrency concern is valid since `getCached()` can be invoked concurrently and there's a potential for inconsistent state between `cachedSaml2Settings` and `lastUsedDashboardsUrl`. However, similar concurrency issues existed prior for `metadataUpdateTime`, so impact is moderate.	Low
General	Avoid static holder for dynamic setting Storing the dynamic setting in a static holder introduces cross-test/state leakage (tests must manually reset it in `finally` blocks) and prevents proper isolation when multiple plugin instances exist. Consider injecting the setting into `HTTPSamlAuthenticator` via a proper mechanism (e.g., constructor or a dedicated service) rather than a static Guice holder. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2599] +private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; - Suggestion importance[1-10]: 3 __ Why: The design concern about static state is legitimate but the suggestion doesn't provide a concrete improved code change (existing and improved code are identical), making it more of a design comment than an actionable suggestion.	Low

Suggestions up to commit fa756db

Category	Suggestion	Impact
Possible issue	Detect null-to-value URL change correctly The Dashboards URL change detection skips the case where `lastUsedDashboardsUrl` is null but `currentDashboardsUrl` is non-null (i.e., URL was set after cache was populated with null). Use `java.util.Objects.equals` to correctly detect all changes including null↔non-null transitions. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [152-158] if (refreshableMetadataResolver.getLastUpdate().isAfter(this.metadataUpdateTime)) { log.debug("IdP metadata has been updated; SAML settings cache will be refreshed"); return true; } // Check if Dashboards URL has changed String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); -if (this.lastUsedDashboardsUrl != null && !this.lastUsedDashboardsUrl.equals(currentDashboardsUrl)) { +if (!java.util.Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { Suggestion importance[1-10]: 7 __ Why: Valid observation: the current check `this.lastUsedDashboardsUrl != null && !this.lastUsedDashboardsUrl.equals(currentDashboardsUrl)` misses the case where `lastUsedDashboardsUrl` is null but `currentDashboardsUrl` becomes non-null, so a runtime update from unset→set would not invalidate the cache. Using `Objects.equals` correctly handles all transitions.	Medium
Possible issue	Invalidate cache on URL change for non-refreshable resolvers When `metadataResolver` is not a `RefreshableMetadataResolver`, `isUpdateRequired` is never called, so cache is never invalidated when the Dashboards URL changes. Ensure the URL-change check runs regardless of resolver type so dynamic URL updates take effect. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [120-135] Saml2Settings getCached() throws SamlConfigException { Instant tempLastUpdate = null; if (this.metadataResolver instanceof RefreshableMetadataResolver && this.isUpdateRequired()) { this.cachedSaml2Settings = null; tempLastUpdate = ((RefreshableMetadataResolver) this.metadataResolver).getLastUpdate(); + } else if (this.cachedSaml2Settings != null + && !java.util.Objects.equals(this.lastUsedDashboardsUrl, this.dashboardsUrlSupplier.get())) { + this.cachedSaml2Settings = null; } if (this.cachedSaml2Settings == null) { String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; this.lastUsedDashboardsUrl = currentDashboardsUrl; } return this.cachedSaml2Settings; } Suggestion importance[1-10]: 7 __ Why: Correct edge case: when the resolver is not a `RefreshableMetadataResolver`, `isUpdateRequired` is never invoked and the URL-change detection is bypassed, so dynamic URL updates would not take effect. The suggestion appropriately extends invalidation to that path.	Medium
General	Make cross-thread static field volatile The `dashboardsUrlSetting` static field is read from authenticator threads while being set from `createComponents`, which introduces a data race. Mark the field `volatile` to ensure visibility across threads (matching common practice for these Guice-held statics). src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [2599] private static PrivilegesConfiguration privilegesConfiguration; -private static OpensearchDynamicSetting<String> dashboardsUrlSetting; +private static volatile OpensearchDynamicSetting<String> dashboardsUrlSetting; Suggestion importance[1-10]: 5 __ Why: Reasonable concern about visibility of the static field across threads; adding `volatile` is a low-risk improvement, though the practical impact is limited since setting occurs once during startup.	Low

Suggestions up to commit f19e2d8

Category	Suggestion	Impact
Possible issue	Detect all URL change transitions The change-detection condition skips the case where `lastUsedDashboardsUrl` is null but `currentDashboardsUrl` is now non-null (e.g., the dynamic setting was set after initial cache with no `kibana_url`). Use `Objects.equals` to correctly detect any transition, including null-to-value. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [150-158] // Check if Dashboards URL has changed String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); -if (this.lastUsedDashboardsUrl != null && !this.lastUsedDashboardsUrl.equals(currentDashboardsUrl)) { +if (!java.util.Objects.equals(this.lastUsedDashboardsUrl, currentDashboardsUrl)) { log.debug( "Dashboards URL has changed from '{}' to '{}'; SAML settings cache will be refreshed", this.lastUsedDashboardsUrl, currentDashboardsUrl ); return true; } Suggestion importance[1-10]: 7 __ Why: Correct observation: the current condition misses null-to-non-null transitions, which is a realistic scenario when the dynamic setting is applied after startup. Using `Objects.equals` fixes this properly.	Medium
Possible issue	Guard against null Dashboards URL If the dynamic setting is cleared and `kibana_url` was never set, `dashboardsUrlSupplier.get()` may return null, causing a `NullPointerException` in `new` `URI(null)` rather than a clear configuration error. Validate the value and throw an informative exception (or return `acsEndpoint` unchanged) when it is null/empty. src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java [404-409] if (acsEndpointUri.isAbsolute()) { return acsEndpoint; } else { String dashboardsUrl = dashboardsUrlSupplier.get(); + if (dashboardsUrl == null \|\| dashboardsUrl.trim().isEmpty()) { + log.error("Dashboards URL is not configured; cannot resolve relative acsEndpoint: {}", acsEndpoint); + return acsEndpoint; + } return new URI(dashboardsUrl).resolve(acsEndpointUri).toString(); } Suggestion importance[1-10]: 6 __ Why: Valid defensive check; if both dynamic setting and `kibana_url` are unset, `new URI(null)` would throw NPE. The fix improves robustness and error clarity, though it's an edge case.	Low
General	Record URL after cache is built Capturing `currentDashboardsUrl` before calling `this.get()` risks recording a stale value if the setting changes during the SAML settings build, causing the cache to appear valid on the next check when it isn't. Capture the URL after the successful build, or capture it once and pass the same value into `get()` to ensure consistency. src/main/java/org/opensearch/security/auth/http/saml/Saml2SettingsProvider.java [128-133] if (this.cachedSaml2Settings == null) { - String currentDashboardsUrl = this.dashboardsUrlSupplier.get(); this.cachedSaml2Settings = this.get(); this.metadataUpdateTime = tempLastUpdate; - this.lastUsedDashboardsUrl = currentDashboardsUrl; + this.lastUsedDashboardsUrl = this.dashboardsUrlSupplier.get(); } Suggestion importance[1-10]: 5 __ Why: Valid race-condition concern about capturing the URL before building settings; the reordering ensures consistency between cached settings and recorded URL, though impact is minor in practice.	Low
General	Ensure thread-safe static setting access `GuiceHolder` fields are static, so `setDashboardsUrlSetting` mutates shared state without synchronization while `getDashboardsUrlSetting` may be called concurrently from authenticator threads. Make the field `volatile` (or use an `AtomicReference`) to ensure visibility across threads. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [1214-1217] +dashboardsUrlSetting.registerClusterSettingsChangeListener(clusterService.getClusterSettings()); +// Make dashboardsUrlSetting available through GuiceHolder for SAML authenticators +GuiceHolder.setDashboardsUrlSetting(dashboardsUrlSetting); Suggestion importance[1-10]: 4 __ Why: Reasonable concern about visibility of static mutable state across threads, but the `improved_code` is identical to `existing_code`, so the suggestion doesn't actually apply the recommended fix.	Low

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-01T15:24:22Z

Persistent review updated to latest commit fa756db

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-01T16:49:34Z

Persistent review updated to latest commit 410b3dd

…ster-setting-support-for-dashboards_url

github-actions · 2026-07-01T16:57:23Z

Persistent review updated to latest commit 008a6b7

codecov · 2026-07-01T18:54:28Z

Codecov Report

❌ Patch coverage is 81.81818% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.01%. Comparing base (96b5429) to head (008a6b7).

Files with missing lines	Patch %	Lines
...nsearch/security/setting/DashboardsUrlSetting.java	57.14%	3 Missing ⚠️
...rity/auth/http/saml/AuthTokenProcessorHandler.java	66.66%	2 Missing ⚠️
...security/auth/http/saml/Saml2SettingsProvider.java	85.71%	2 Missing ⚠️
...security/auth/http/saml/HTTPSamlAuthenticator.java	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6275      +/-   ##
==========================================
- Coverage   75.02%   75.01%   -0.01%     
==========================================
  Files         451      452       +1     
  Lines       29248    29285      +37     
  Branches     4407     4411       +4     
==========================================
+ Hits        21942    21968      +26     
- Misses       5267     5279      +12     
+ Partials     2039     2038       -1

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`85.23% <100.00%> (+0.12%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.65% <ø> (ø)`
...security/auth/http/saml/HTTPSamlAuthenticator.java	`69.19% <90.00%> (+0.77%)`	⬆️
...rity/auth/http/saml/AuthTokenProcessorHandler.java	`51.44% <66.66%> (-0.32%)`	⬇️
...security/auth/http/saml/Saml2SettingsProvider.java	`62.50% <85.71%> (+2.31%)`	⬆️
...nsearch/security/setting/DashboardsUrlSetting.java	`57.14% <57.14%> (ø)`

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

github-actions · 2026-07-05T17:25:24Z

Persistent review updated to latest commit afbf218

…dashboards_url

github-actions · 2026-07-08T05:12:01Z

Persistent review updated to latest commit 33b24e7

Add support for dynamic Dashboards URL via cluster settings

f19e2d8

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

praveenvenkat06 requested review from DarshitChanpura, Rishav9852Kumar, RyanL1997, cwperks, derek-ho, finnegancarroll, nibix, reta, shikharj05 and willyborankin as code owners July 1, 2026 10:52

praveenvenkat06 marked this pull request as draft July 1, 2026 10:53

JUnits for dynamic Dashboards URL implementation

fa756db

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

Implement PR code suggestions

410b3dd

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

Merge remote-tracking branch 'upstream/main' into feature/dynamic-clu…

008a6b7

…ster-setting-support-for-dashboards_url

praveenvenkat06 marked this pull request as ready for review July 1, 2026 16:55

Rishav9852Kumar reviewed Jul 2, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/auth/http/saml/AuthTokenProcessorHandler.java

Update AuthTokenProcessorHandler.java

afbf218

Signed-off-by: Praveen Raj V <praveenvenkat06@gmail.com>

praveenvenkat06 requested a review from Rishav9852Kumar July 6, 2026 04:46

Merge branch 'main' into feature/dynamic-cluster-setting-support-for-…

33b24e7

…dashboards_url

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for dynamic Dashboards URL via cluster settings - #6275

Add support for dynamic Dashboards URL via cluster settings#6275
praveenvenkat06 wants to merge 6 commits into
opensearch-project:mainfrom
praveenvenkat06:feature/dynamic-cluster-setting-support-for-dashboards_url

praveenvenkat06 commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

codecov Bot commented Jul 1, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

praveenvenkat06 commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 33b24e7)

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

codecov Bot commented Jul 1, 2026

Codecov Report

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

praveenvenkat06 commented Jul 1, 2026 •

edited

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading

(Review updated until commit `33b24e7`)

github-actions Bot commented Jul 1, 2026 •

edited

Loading